Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[App Search] Add roleHasScopedEngines helper + small roles/ cleanup #94038

Merged
merged 2 commits into from
Mar 9, 2021

Conversation

cee-chen
Copy link
Contributor

@cee-chen cee-chen commented Mar 9, 2021

Summary

@scottybollinger needs this helper for his App Search role mappings work.

Thanks to his great suggestion in #94032 (comment), I've moved this out of myRole to its own separate utility fn, and split up the role/ folder some (into separate utils+types)

Checklist

@cee-chen cee-chen added Feature:Plugins release_note:skip Skip the PR/issue when compiling release notes v7.13.0 auto-backport Deprecated - use backport:version if exact versions are needed labels Mar 9, 2021
@cee-chen cee-chen requested a review from a team March 9, 2021 01:51
Comment on lines +13 to +16
export const roleHasScopedEngines = (roleType: RoleTypes): boolean => {
const unscopedRoles = ['dev', 'editor', 'analyst'];
return unscopedRoles.includes(roleType);
};
Copy link
Contributor Author

@cee-chen cee-chen Mar 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the main new added utility/functionality (ported over from https://github.com/elastic/ent-search/blob/master/app/javascript/app_search/classes.ts#L10-L13)

Comment on lines +13 to +16
* Transforms the `role` data we receive from the Enterprise Search
* server into a more convenient format for front-end use
*/
export const getRoleAbilities = (role: Account['role']): Role => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fn is the exact same as before, I just moved the type definitions out into types.ts. The types are also the same as before

@cee-chen cee-chen requested a review from scottybollinger March 9, 2021 02:48
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
enterpriseSearch 1308 1311 +3

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
enterpriseSearch 2.1MB 2.1MB +1.2KB

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Copy link
Contributor

@scottybollinger scottybollinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for taking the extra time!


import { roleHasScopedEngines } from './';

describe('roleHasScopedEngines()', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do not make this update for this PR, it's not worth running CI. Just noting that we usually don't add parens to the end of function names in describes.

Suggested change
describe('roleHasScopedEngines()', () => {
describe('roleHasScopedEngines', () => {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

haha thanks Jason, I was totally about to smash commit suggestion before seeing the do not (italics totally worked). This is an accidental copypaste carry over from

  • I think I originally added the () because it's a function helper within a 'class'/obj that has static vars mixed in with fn's 🤔 not sure if the () is helpful in those scenarios - agreed they're not helpful for utility files where everything is a function

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@scottybollinger if you're down, feel free to make Jason's suggested copy change in a future role mappings PR!

@cee-chen cee-chen merged commit ee2ec0a into elastic:master Mar 9, 2021
@cee-chen cee-chen deleted the roleHasScopedEngines branch March 9, 2021 16:54
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Mar 9, 2021
…elastic#94038)

* Split out roles/ into separate files

- in preparation for new scoped engines helper

* Add new roleHasScopedEngines helper
@kibanamachine
Copy link
Contributor

💚 Backport successful

7.x / #94144

Successful backport PRs will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Mar 9, 2021
…#94038) (#94144)

* Split out roles/ into separate files

- in preparation for new scoped engines helper

* Add new roleHasScopedEngines helper

Co-authored-by: Constance <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:Plugins release_note:skip Skip the PR/issue when compiling release notes v7.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants